hw-isolation rebase: Added guard, deconfiguration, sparecore #1279

gcpin · 2025-05-07T10:59:49Z

1. Add support for guard, deconfiguration and sparecore
2. hw-isolation code relocation to systems_logservices_hwisolation.hpp

Following commits from 1110 are used to create this unified commit

332c310625 - redfish-core: LogServices: Added HardwareIsolation service
36dec57033 - LogServices: HardwareIsolation: LogEntryCollection
3486c6abca - LogServices: HardwareIsolation: Get LogEntry
3d981efb6a - LogServices: HardwareIsolation: Delete LogEntry
11e4cda449 - LogServices: HardwareIsolation: Post ClearLog
723c0fdc70 - redfish-core: Core: Enabled the isolation feature
e4cb16309a - redfish-core: Memory:Enabled the isolation feature
e2158ab941 - Core: Fix, Patch a core into the respective parent processor
4237af36b9 - Hw-Isolation: Deconfiguration type for DIMMs
21582d8c65 - Enabled deconfiguration reason support to the DIMM
558b32f3cc - HW-Isolation: Fix, Don't throw internal error if failed to get error log
2ae01eaf3b - HW-Isolation: Fix, Update State if the Core and DIMM are recovered
84aef9b900 - HW-Isolation: Return an appropriate error if the request is failed.
9ac66c52ac - HW-Isolation: Return ResourceCannotBeDeleted error
f769499d62 - LogService: HW-Isolation: Return an appropriate error
c3839dba53 - redfish-core: Core: Enabled the isolation (aka guard) feature
555fc6ca4c - HW-Isolation: Fix, Use GetAncestors to get the parents id
2cf7c3f830 - HW-Isolation: Fill OriginOfCondition for the TPM and Motherboard
3bd202e472 - LogEntry: HW-Isolation: Removed the Resolved property
10ed0438b8 - HardwareIsolation: Spare core support
5e53b61d8a - HW-Isolation: Fix info PEL AdditionalDataURI
50c9dc3076 - HW-Isolation: Fix Additional Data Uri
7adcfa13f1 - Move getHiddenPropertyValue method to utils
b90a99f053 - Hw-Isolation: Fix eventId HW Deconfiguration page

jenkins-openbmc-ibm · 2025-05-07T11:04:02Z

Can one of the admins verify this patch?

gtmills · 2025-05-08T13:59:25Z

add to approvelist

baemyung · 2025-05-08T14:45:51Z

redfish-core/include/utils/error_log_utils.hpp

+            asyncResp->res.jsonValue[errorLogPropPath]["@odata.id"] =
+                boost::urls::format(
+                    "/redfish/v1/Systems/system/LogServices/{}/Entries/{}{}",
+                    logPath, entryID, linkAttachment);


I think this is better to be

asyncResp->res.jsonValue[errorLogPropPath]["@odata.id"] = boost::urls::format( "/redfish/v1/Systems/{}/LogServices/{}/Entries/{}{}", BMCWEB_REDFISH_SYSTEM_URI_NAME, logPath, entryID, linkAttachment);

done. Will push the changes together shortly

baemyung · 2025-05-08T14:47:11Z

redfish-core/include/utils/hw_isolation.hpp

@@ -0,0 +1,690 @@
+#pragma once


License statement

// SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright OpenBMC Authors #pragma once

done. Will push the changes together shortly

baemyung · 2025-05-08T14:48:42Z

redfish-core/include/utils/hw_isolation.hpp

+#include <sdbusplus/message.hpp>
+#include <sdbusplus/message/native_types.hpp>
+#include <utils/error_log_utils.hpp>
+#include <utils/time_utils.hpp>


I think the above 2 include files would need to be (moved to the section L2-L7)

#include "utils/error_log_utils.hpp" #include "utils/time_utils.hpp"

done. Will push the changes together shortly

baemyung · 2025-05-08T14:52:04Z

redfish-core/include/utils/hw_isolation.hpp

+        },
+        "xyz.openbmc_project.ObjectMapper",
+        "/xyz/openbmc_project/object_mapper",
+        "xyz.openbmc_project.ObjectMapper", "GetSubTreePaths",


This is better to be done with

dbus::utility::getSubTreePaths(...)

baemyung · 2025-05-08T14:55:42Z

redfish-core/include/utils/hw_isolation.hpp

+                            // the respective hardware is not functional so
+                            // set the state as "Disabled".
+                            asyncResp->res.jsonValue["Status"]["State"] =
+                                "Disabled";


asyncResp->res.jsonValue["Status"]["State"] = resource::State::Disabled;

done. Will push the changes together shortly

baemyung · 2025-05-08T16:03:01Z

redfish-core/lib/memory.hpp

+inline void getObjectEnable(std::shared_ptr<bmcweb::AsyncResp> asyncResp,
+                            const std::string& service, const std::string& path)
+{
+    sdbusplus::asio::getProperty<bool>(


Use

dbus::utility::getProperty<bool>()

baemyung · 2025-05-08T16:03:18Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+        }
+        return;
+    }
+    sdbusplus::asio::getProperty<std::string>(


Use

dbus::utility::getProperty<bool>()

baemyung · 2025-05-08T16:03:40Z

redfish-core/include/utils/error_log_utils.hpp

+    const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
+    const std::string& entryId, std::function<void(bool hidden)>&& callback)
+{
+    sdbusplus::asio::getProperty<bool>(


Use

dbus::utility::getProperty<bool>()

baemyung · 2025-05-08T16:04:29Z

redfish-core/include/utils/hw_isolation.hpp

+                    }
+
+                    // Get event properties and fill into status conditions
+                    sdbusplus::asio::getAllProperties(


Use

dbus::utility::getAllProperties

baemyung · 2025-05-08T16:11:58Z

redfish-core/lib/log_services.hpp

+                                   "LogServices/HardwareIsolation"}});
+                asyncResp->res.jsonValue["[email protected]"] =
+                    logServiceArrayLocal.size();
+            }


This would be better like to in the older #1256

if constexpr (BMCWEB_HW_ISOLATION) { nlohmann::json& logServiceArrayLocal = asyncResp->res.jsonValue["Members"]; nlohmann::json::object_t member; member["@odata.id"] = boost::urls::format( "redfish/v1/Systems/{}/LogServices/HardwareIsolation", BMCWEB_REDFISH_SYSTEM_URI_NAME); logServiceArrayLocal.emplace_back(std::move(member)); asyncResp->res.jsonValue["[email protected]"] = logServiceArrayLocal.size(); }

jeaaustx · 2025-05-08T18:42:07Z

redfish-core/include/utils/hw_isolation.hpp

+    crow::connections::systemBus->async_method_call(
+        [asyncResp, resourceObjPath](
+            const boost::system::error_code& ec,
+            const dbus::utility::MapperGetAncestorsResponse& ancestors) {


I realize this code is being carried forward from prior releases. But in its current format it goes against the OpenBMC coding standards. Specifically using long lambdas. Suggest cleaning this up to move into a named function instead.

(Same comment applies throughout this file and much of this review.)

We will defer this. Created another JIRA to address moving of lambdas to function. Will attach it to the main conversation

jeaaustx · 2025-05-08T18:43:38Z

redfish-core/include/utils/hw_isolation.hpp

+                // for parent chassis object id alone, so we should get one
+                // element.
+                BMCWEB_LOG_ERROR(
+                    "The given resource object [{}] is contains more than one Chassis as parent so failed return ChassisPowerStateOffRequiredError in the response",


extraneous word: "is"

jeaaustx · 2025-05-08T20:18:42Z

redfish-core/include/utils/hw_isolation.hpp

+    if (severityVal ==
+        "xyz.openbmc_project.Logging.Event.SeverityLevel.Critical")
+    {
+        asyncResp->res.jsonValue[severityPropPath] = "Critical";


Should these strings map to generated Redfish enums?

jeaaustx · 2025-05-08T20:26:49Z

redfish-core/include/utils/hw_isolation.hpp

+        // MessageRegistries
+        std::vector<std::string> messageArgs{*msgPropVal};
+
+        // Fill the "msgPropVal" as reason


Can this use the utility function fillMessageArgs() instead?

Will also address this as part of the new JIRA

jeaaustx · 2025-05-08T20:47:32Z

redfish-core/lib/memory.hpp

+ * @brief API used to process the Memory "Enabled" member which is
+ *        patched to do appropriate action.
+ *
+ * @param[in] resp - The redfish response to return.


nit: comment doesn't match name of parameter

jeaaustx · 2025-05-08T21:21:47Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+    crow::connections::systemBus->async_method_call(
+        [asyncResp, dbusObjPath,
+         entryJsonIdx](const boost::system::error_code& ec,
+                       const dbus::utility::MapperGetObject& objType) {


This is a really long lambda. It would be better to put it into a named function.

Will address this in a separate commit. Attached JIRA ticket for reference

jeaaustx · 2025-05-08T21:23:19Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+            }
+
+            // Fill the isolated hardware object id along with the Redfish URI
+            std::string redfishUri =


Better to use std::format() here.

jeaaustx · 2025-05-08T21:30:46Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+                        severityString ==
+                            "xyz.openbmc_project.HardwareIsolation.Entry.Type.Spare")
+                    {
+                        entryJson["Severity"] = "OK";


Could this use a generated Redifsh enum?

jeaaustx · 2025-05-08T21:35:11Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+    crow::connections::systemBus->async_method_call(
+        [asyncResp, entryId,
+         entryObjPath](const boost::system::error_code& ec,
+                       const dbus::utility::MapperGetObject& objType) {


This file also has lots of long lambdas which would benefit the readability to move into named functions per the coding guidelines.

Will address in the next commit. Attached JIRA ticket details for reference

jeaaustx · 2025-05-08T21:37:31Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+        .methods(boost::beast::http::verb::get)(
+            getSystemHardwareIsolationLogEntryCollection);
+
+    BMCWEB_ROUTE(app, "/redfish/v1/Systems/system/LogServices/"


It would be better for searching the code if the Redfish URI path string was all in a single string. Same comment below.

gtmills · 2025-05-14T17:36:46Z

jenkins run tests please

deepakala-k

Thanks for all the comments. I have addressed almost all of them except that ones that include changes to convert the lambdas into named function. Please review it. I will get the JIRA details from my team and attach it shortly. (by our tomorrow morning)

deepakala-k · 2025-05-19T13:08:16Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+            }
+
+            // Fill the isolated hardware object id along with the Redfish URI
+            std::string redfishUri =


deepakala-k · 2025-05-19T13:09:07Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+        }
+        return;
+    }
+    sdbusplus::asio::getProperty<std::string>(


deepakala-k · 2025-05-20T10:21:02Z

redfish-core/include/utils/error_log_utils.hpp

@@ -0,0 +1,82 @@
+#pragma once


deepakala-k · 2025-05-21T16:37:57Z

redfish-core/include/utils/hw_isolation.hpp

+    crow::connections::systemBus->async_method_call(
+        [asyncResp, resourceObjPath](
+            const boost::system::error_code& ec,
+            const dbus::utility::MapperGetAncestorsResponse& ancestors) {


We will defer this. Created another JIRA to address moving of lambdas to function. Will attach it to the main conversation

deepakala-k · 2025-05-21T16:38:45Z

redfish-core/include/utils/hw_isolation.hpp

+                },
+                "xyz.openbmc_project.ObjectMapper",
+                "/xyz/openbmc_project/object_mapper",
+                "xyz.openbmc_project.ObjectMapper", "GetObject",


deepakala-k · 2025-05-21T16:44:42Z

redfish-core/include/utils/hw_isolation.hpp

+    if (severityVal ==
+        "xyz.openbmc_project.Logging.Event.SeverityLevel.Critical")
+    {
+        asyncResp->res.jsonValue[severityPropPath] = "Critical";


deepakala-k · 2025-05-21T16:52:00Z

redfish-core/include/utils/hw_isolation.hpp

+                },
+                "xyz.openbmc_project.ObjectMapper",
+                "/xyz/openbmc_project/object_mapper",
+                "xyz.openbmc_project.ObjectMapper", "GetObject",


deepakala-k · 2025-05-21T16:54:26Z

redfish-core/lib/log_services.hpp

+                                   "LogServices/HardwareIsolation"}});
+                asyncResp->res.jsonValue["[email protected]"] =
+                    logServiceArrayLocal.size();
+            }


deepakala-k · 2025-05-21T16:55:15Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+    crow::connections::systemBus->async_method_call(
+        [asyncResp, dbusObjPath,
+         entryJsonIdx](const boost::system::error_code& ec,
+                       const dbus::utility::MapperGetObject& objType) {


Will address this in a separate commit. Attached JIRA ticket for reference

deepakala-k · 2025-05-21T16:56:01Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+    crow::connections::systemBus->async_method_call(
+        [asyncResp, entryId,
+         entryObjPath](const boost::system::error_code& ec,
+                       const dbus::utility::MapperGetObject& objType) {


Will address in the next commit. Attached JIRA ticket details for reference

baemyung · 2025-05-21T18:25:13Z

redfish-core/lib/processor.hpp

+            {
+                getEnabledStatus(asyncResp, service, corePath, intf);
+            }
+#ifdef BMCWEB_ENABLE_HW_ISOLATION


Use

if constexpr (BMCWEB_HW_ISOLATION) { // Check for the hardware status event hw_isolation_utils::getHwIsolationStatus(asyncResp, corePath); }

baemyung · 2025-05-21T18:27:10Z

redfish-core/lib/processor.hpp

+        }
+    };
+
+    getProcessorPaths(asyncResp, processorId, std::move(callback));


Since callback is small, I think it is better to be inlined as a lambda body

getProcessorPaths(asyncResp, processorId, [asyncResp, coreId, enabled](const std::string& cpuPath) { // Handle patched Enabled Redfish property if (enabled.has_value()) { patchCpuCoreMemberEnabled(asyncResp, cpuPath, coreId, *enabled); });

baemyung · 2025-05-21T18:33:20Z

include/dbus_utility.hpp

@@ -179,6 +179,11 @@ void getDbusObject(const std::string& path,
                   std::function<void(const boost::system::error_code&,
                                      const MapperGetObject&)>&& callback);

+void getAncestor(


Since it is matching with "GetAncestors" dbus-call, I think it is better to be named as getAncestors().

baemyung · 2025-05-21T18:37:05Z

redfish-core/lib/assembly.hpp

+                    // Assembly
+                    // GET and
+                    // PATCH
+                    // handler.


Please merge the above comment line L838-L852 into a line and rerun `clang-format-19' to squeeze them

baemyung · 2025-05-21T18:42:09Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+                    // name,
+                    // object
+                    // path, and
+                    // interface


Please merge the comment of lines L333-L340 (and then rerun clang-format-19)

baemyung · 2025-05-21T18:44:44Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+    BMCWEB_ROUTE(
+        app,
+        "/redfish/v1/Systems/<str>/LogServices/HardwareIsolation/Actions/LogService.ClearLog/")
+        .privileges(redfish::privileges::postLogService)


This will be

.privileges(redfish::privileges:: postLogServiceSubOverComputerSystemLogServiceCollection)

baemyung · 2025-05-21T18:45:09Z

src/dbus_utility.cpp

@@ -224,6 +224,22 @@ void getDbusObject(const std::string& path,
        "xyz.openbmc_project.ObjectMapper", "GetObject", path, interfaces);
 }

+void getAncestor(


getAncestors()

baemyung · 2025-05-21T18:55:34Z

redfish-core/lib/assembly.hpp

+            // implemented before
+            // finding the assembly id
+            // as per bmcweb Assembly
+            // design.


Also combine the above L787-L793 comment lines

``` 1. Add support for guard, deconfiguration and sparecore 2. hw-isolation code relocation to systems_logservices_hwisolation.hpp Following commits from 1110 are used to create this unified commit 332c310 - redfish-core: LogServices: Added HardwareIsolation service 36dec57 - LogServices: HardwareIsolation: LogEntryCollection 3486c6a - LogServices: HardwareIsolation: Get LogEntry 3d981ef - LogServices: HardwareIsolation: Delete LogEntry 11e4cda - LogServices: HardwareIsolation: Post ClearLog 723c0fd - redfish-core: Core: Enabled the isolation feature e4cb163 - redfish-core: Memory:Enabled the isolation feature e2158ab - Core: Fix, Patch a core into the respective parent processor 4237af3 - Hw-Isolation: Deconfiguration type for DIMMs 21582d8 - Enabled deconfiguration reason support to the DIMM 558b32f - HW-Isolation: Fix, Don't throw internal error if failed to get error log 2ae01ea - HW-Isolation: Fix, Update State if the Core and DIMM are recovered 84aef9b - HW-Isolation: Return an appropriate error if the request is failed. 9ac66c5 - HW-Isolation: Return ResourceCannotBeDeleted error f769499 - LogService: HW-Isolation: Return an appropriate error c3839db - redfish-core: Core: Enabled the isolation (aka guard) feature 555fc6c - HW-Isolation: Fix, Use GetAncestors to get the parents id 2cf7c3f - HW-Isolation: Fill OriginOfCondition for the TPM and Motherboard 3bd202e - LogEntry: HW-Isolation: Removed the Resolved property 10ed043 - HardwareIsolation: Spare core support 5e53b61 - HW-Isolation: Fix info PEL AdditionalDataURI 50c9dc3 - HW-Isolation: Fix Additional Data Uri 7adcfa1 - Move getHiddenPropertyValue method to utils b90a99f - Hw-Isolation: Fix eventId HW Deconfiguration page ``` Signed-off-by: Gopichand Paturi <[email protected]> Co-authored-by: Ramesh Iyyar <[email protected]> Co-authored-by: deepakala k <[email protected]>

deepakala-k · 2025-05-22T07:43:35Z

Thank you reviewing with close attention. I have addressed all the comments.

baemyung

It looks much better and I think it can go in.
(Later more to be refined and enhanced).

deepakala-k · 2025-05-22T12:44:51Z

@baemyung I captured all the comments that are yet to be addressed in this JIRA. https://jsw.ibm.com/browse/PFEBMC-4397
Thanks a lot for the quick review.

baemyung · 2025-05-22T16:07:41Z

redfish-core/lib/memory.hpp

+                return;
+            }
+
+            asyncResp->res.jsonValue["Enabled"] = enabled;


This "Enabled" field requires Memory schema version v1_12_0.

https://ibm-systems-power.slack.com/archives/C04KG79NAUA/p1747927965504969?thread_ts=1747903373.957939&cid=C04KG79NAUA

baemyung · 2025-05-22T16:11:06Z

redfish-core/lib/log_services.hpp

+                    asyncResp->res.jsonValue["Members"];
+                nlohmann::json::object_t member;
+                member["@odata.id"] = boost::urls::format(
+                    "redfish/v1/Systems/{}/LogServices/HardwareIsolation",


https://ibm-systems-power.slack.com/archives/C04KG79NAUA/p1747927965504969?thread_ts=1747903373.957939&cid=C04KG79NAUA

The validator fails because of a missing "/" in front of this.

member["@odata.id"] = boost::urls::format( "/redfish/v1/Systems/{}/LogServices/HardwareIsolation",

gtmills requested review from gtmills, baemyung and jeaaustx May 8, 2025 13:58

baemyung reviewed May 8, 2025

View reviewed changes

jeaaustx reviewed May 8, 2025

View reviewed changes

gcpin changed the title ~~WIP: hw-isolation rebase: Added guard, deconfiguration, sparecore~~ hw-isolation rebase: Added guard, deconfiguration, sparecore May 13, 2025

deepakala-k force-pushed the bmcweb_hwisolation_rebase branch 8 times, most recently from 24457ee to 7512534 Compare May 21, 2025 16:57

deepakala-k reviewed May 21, 2025

View reviewed changes

deepakala-k requested review from baemyung and jeaaustx May 21, 2025 17:00

baemyung reviewed May 21, 2025

View reviewed changes

deepakala-k force-pushed the bmcweb_hwisolation_rebase branch from 7512534 to bcebe75 Compare May 22, 2025 07:38

deepakala-k requested a review from baemyung May 22, 2025 07:42

baemyung approved these changes May 22, 2025

View reviewed changes

rfrandse merged commit 7139c83 into ibm-openbmc:1120 May 22, 2025
1 check passed

baemyung reviewed May 22, 2025

View reviewed changes

deepakala-k mentioned this pull request May 23, 2025

hw-isolation rebase: Added guard, deconfiguration, sparecore #1297

Merged

hw-isolation rebase: Added guard, deconfiguration, sparecore #1279

hw-isolation rebase: Added guard, deconfiguration, sparecore #1279

Uh oh!

Conversation

gcpin commented May 7, 2025

Uh oh!

jenkins-openbmc-ibm commented May 7, 2025

Uh oh!

gtmills commented May 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gtmills commented May 14, 2025

Uh oh!

deepakala-k left a comment

Choose a reason for hiding this comment